-
Notifications
You must be signed in to change notification settings - Fork 0
Update to 2.14.0, testframework changes, singlelinesection updates #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to 2.14.0, testframework changes, singlelinesection updates #8
Conversation
…to work with native test suite
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe PR simplifies test identifier semantics from script-qualified format to test-name-only identifiers across documentation and test framework declarations. It refactors the parser lifecycle in the single-line section handler, updates lambda argument passing in functions, and adds test coverage for new syntax patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/functionsv2.sk (2)
104-106: Import statement placed mid-file.The import for
java.net.URLis added after test definitions. While this may work in Skript, consolidating imports at the top of the file (with the existingjava.util.HashMapimport on line 2) improves readability and follows conventional import organization.Suggested refactor
Move the import to the top of the file:
import: java.util.HashMap + java.net.URLThen remove lines 104-106.
107-110: Assertion is missing an error message.The assertion on line 110 lacks a
with "<message>"clause, which is inconsistent with other assertions in this file. If the assertion fails, the error output will be less informative.Suggested fix
- assert {_val}.getClass().getSimpleName().equalsIgnoreCase("URL") is true + assert {_val}.getClass().getSimpleName().equalsIgnoreCase("URL") is true with "java call type check failed"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/testframework.mdscripts/libs/functionsv2.skscripts/libs/singlelinesection.skscripts/utils/testframework.sktests/functionsv2.sktests/singlinesection.sk
🔇 Additional comments (11)
tests/singlinesection.sk (2)
33-35: Variable{_b}is used but not set in this section.The section sets
{_a}to 2 and{_c}to 3, then returns{_a} + {_b} + {_c}. However,{_b}is not assigned within this section. The expected result of 5 implies{_b}carries over a value from the previous test (line 30 sets{_b}to 2).If this is intentional to test variable context inheritance between section runs, consider adding a comment to clarify. If not, this test may produce unexpected results if run in isolation.
28-31: New multi-statement section test looks correct.The test validates that statements separated by
%nl%are executed sequentially, with{_a}=1,{_b}=2, and the return value being their sum (3). This aligns with the implementation changes inscripts/libs/singlelinesection.sk.scripts/libs/functionsv2.sk (2)
73-74: Argument passing changed to string interpolation format.The change from runtime arg references to string-quoted arguments (
"%{proxy}%","%{arg}%") alters how arguments are resolved. This appears intentional for the noreturn consumer/biconsumer paths.
106-108: Inner lambda script context fallback.The fallback to
current scriptwhen the script reference cannot be resolved from the origin node handles nested lambda scenarios where the config filename is unavailable.scripts/utils/testframework.sk (2)
14-20: Test event pattern and registration updated for test-name-only semantics.The event pattern now accepts an optional
[devdinc]prefix, and test registration stores identifiers using only the test name. This aligns with the documented shift away from script-qualified identifiers.
242-326: Internal framework tests prefixed withdevdinc.All internal test declarations consistently use the new
devdincprefix. The test logic and assertions remain functionally equivalent.docs/testframework.md (1)
77-78: Documentation updated to reflect test-name-only identifier semantics.The documentation correctly reflects the implementation changes: test identifiers are now based solely on test names rather than script-qualified paths. Storage format, discovery expressions, and isolation guarantees are all updated consistently.
Also applies to: 92-93, 125-126, 135-136, 355-356
scripts/libs/singlelinesection.sk (4)
46-58: Parser lifecycle properly preserves and restores state.The new implementation correctly captures the original
isActivestate, performs the parse operations, then restores both the backup and original active state. This is cleaner than the previous approach and ensures proper state management for nested parsing scenarios.
62-62: Expression signature updated for multi-statement support.The pattern change from
compile line %string%to[new] [single] line section %strings%supports the new multi-statement capability where expr-1 can be multiple strings joined with newlines.
72-83: String-based argument extraction path.When
expr-2is a String instance (lines 72-79), the code extracts variable names by stripping curly braces and creating Variable instances. This handles the case where arguments are passed as string literals like"{_x}"rather than raw variable references.The logic correctly differentiates between string-based and raw expression-based argument passing, routing to the appropriate extraction function.
84-86: Multi-statement code generation via newline join.The change to
join expr-1 with nlenables multi-statement sections where each string in expr-1 becomes a separate line. This aligns with the new test cases intests/singlinesection.sk.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.